-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Basic Authentication Scheme #5
Conversation
would be based64 encoded and placed in the authorization header as follows: | ||
|
||
``` | ||
Authorization: Basic a3lsZTpiMjk1MmQwM2JkYTA5Y2I1ZjYzYjAxNjJmYmJlZTc3Yw== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this is very similar to the OAuth problem mentioned in the other PR. Basically you have a field that is made up of a constant and some computed value.
👍 Looks good. |
As an anonymous scheme: | ||
|
||
```apib | ||
+ Basic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this ought be + Authentication (Basic)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ Authenticated (Basic)
to be exact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bas been addressed.
Looks good but the #5 (comment) |
Ping @kylef this is good to go once https://github.com/apiaryio/api-blueprint-rfcs/pull/5/files#r42629521 is addressed. |
|
||
As such, a basic authentication scheme may configure two properties, | ||
`username` and `password`. These properties indicate a sample username and | ||
password that may be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly are we configuring here? We say properties. But we are actually configuring sample username and sample password. I don't see any mention about the values being an example.
Also, I think we should have a default example username and example password for Basic
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the MSON Specification, 2.3.1 Property Member Types, the properties within an MSON object are called "Properties". The wording here reflects that.
These are properties within the authentication scheme's object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the sample/example, I'm not sure I follow. What would the difference between sample and example be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention the default values for the properties username
and password
, then this looks good.
My review is done. I have a #5 (comment) which needs to be clarified for this to move forward. |
7154566
to
ded953d
Compare
Reviewed the latest changes. |
ded953d
to
8c05c33
Compare
@pksunkara I've updated this pull request now. |
``` | ||
|
||
There are no default values for the sample `username` and `password`. When | ||
these are both unset, there is no sample credentials. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kylef Are we sure about this? Some users might want to directly use Basic
simply like this:
+ Authenticated (Basic)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes they can, just because there are no defaults, doesn't mean you can't indicate that a resource/group/action/request example etc supports the basic scheme. It just represents there are not sample credentials available.
You don't need to define a sample username
or password
to use this scheme.
Does this make sense? Is there something I can add to make this a little clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to define a sample username or password to use this scheme.
I understand and I agree. But we also need to think about the tools which use API Blueprint. Dredd, Documentation and others like them. Let's take the example of Dredd. User uses the Basic
directly for an action. But what values will dredd use now when testing the server? If we leave this upto the tools, each one of them will have their own default values and user will start getting overwhelmed. Having a default sample username
and password
will avoid all this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if there is no sample values, a user should have to supply the value to use at run-time. For example:
$ dredd --user kyle:password
I disagree that we should require services to provide a sample account which is described in the blueprint (and even dictating the default username/password).
/cc @netmilk -- would love to hear your thoughts around this matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dredd supports basic auth already by supplying the user and password from command line as described above by @kylef and I don't see a reason why we shouldn't keep it that way. I think the mandatory sample account is not necessary. Blueprint doesn't require defaults on anything so why it should all of a sudden require it it doesn't make sense. Sure I would push users to provide values as a convenient way how to do things but not force them to.
👍 |
This pull request extends on the authentication framework (proposed in #4) to provide a base authentication scheme called "Basic".
Dependencies: